Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] Provide rules to migrate EXT:vhs viewhelpers #3424

Merged
merged 8 commits into from
Feb 5, 2024

Conversation

kitzberger
Copy link
Contributor

@kitzberger kitzberger commented Jun 6, 2023

  • v:extension.path.relative -> f:uri.resource
  • v:format.json.encode -> f:format.json
  • v:media.image -> f:media.image
  • v:uri.image -> f:uri.image
  • v:l -> f:translate
  • v:variable.set -> f:variable
  • v:or -> inline ternary operator

t.b.c.

@kitzberger kitzberger marked this pull request as draft June 6, 2023 13:18
@kitzberger kitzberger force-pushed the replace-vhs branch 3 times, most recently from 864c0cd to cb94277 Compare June 6, 2023 13:52
@kitzberger kitzberger changed the title [FEATURE] Provide rule to migrate EXT:vhs viewhelpers [FEATURE] Provide rules to migrate EXT:vhs viewhelpers Jun 6, 2023
@helsner
Copy link
Collaborator

helsner commented Jun 14, 2023

Nice draft, a note though:
The v:variable.set -> f:variable change could cause issues, when used with multi dimensional
so v:variable.set name="foo.bar" won't work with f:variable name="foo.bar" from my experience

@sabbelasichon sabbelasichon marked this pull request as ready for review June 14, 2023 10:21
@sabbelasichon
Copy link
Owner

@kitzberger Nice. What we will also need are tests. Could you provide them?

@kitzberger
Copy link
Contributor Author

@kitzberger Nice. What we will also need are tests. Could you provide them?

@sabbelasichon, like this? Any suggestions? I know, one is failing. I need to figure out how to migrate single viewhelper properties.

@kitzberger
Copy link
Contributor Author

Nice draft, a note though: The v:variable.set -> f:variable change could cause issues, when used with multi dimensional so v:variable.set name="foo.bar" won't work with f:variable name="foo.bar" from my experience

@helsner, good point. Is there a way of migrating the code and print some warning in such a case? Or is that counter-productive since nobody every reads them anyways?

@helsner
Copy link
Collaborator

helsner commented Jun 14, 2023

I would say there is no proper way of migration as the logic behind the var cannot be further checked (especially in HTML).
A warning should be possible and the prevention of changing the viewhelper call in case there is a dot within the name mandatory.
As we don't change the code then, it should already be good, in case the warning is ignored we don't destroy functionality and I cannot help people that ignore our notice 🤷

@kitzberger
Copy link
Contributor Author

Do you know of any examples where a warning is being output? Thanks for your input :-)

@helsner
Copy link
Collaborator

helsner commented Jun 14, 2023

Do you know of any examples where a warning is being output? Thanks for your input :-)

simply look for rectorOutputStyle in existing rules.
It's basically used via constructor and ...->warning() call most of the times

@kitzberger
Copy link
Contributor Author

kitzberger commented Jun 15, 2023

@helsner, thanks for the hint!

I've now added a more strict pattern to detect the simple cases like: v:variable.set name="xx" and migrate only those automatically. Any left occurrences of v:variable.set I keep untouched and print a warning instead.

What do you think?

@helsner
Copy link
Collaborator

helsner commented Jun 15, 2023

@kitzberger nice, but i would point out to why the change is not made as well, as this might not be known (i found that out the hard way 😂 )

@helsner
Copy link
Collaborator

helsner commented Jun 22, 2023

This still looks great, but i just checked and noticed that we have some TODO's left

@mehdichaouch
Copy link

This still looks great, but i just checked and noticed that we have some TODO's left

Nice PR, do you know when it'll be ready to merge?

Thanks,

@kitzberger kitzberger force-pushed the replace-vhs branch 2 times, most recently from 1d5230b to ca8f57c Compare July 17, 2023 09:42
@sabbelasichon
Copy link
Owner

@helsner What kind of TODO´s are left here?

@helsner
Copy link
Collaborator

helsner commented Jul 31, 2023

@helsner What kind of TODO´s are left here?

Those marked in the code I guess
Maybe we can still merge and they will be added afterwards
But usually we migrate fully because it would cause too much frustration not handling attributes etc

@sabbelasichon
Copy link
Owner

@helsner If you like you can merge it. I am not using VHS anyways ;-)

@helsner
Copy link
Collaborator

helsner commented Aug 10, 2023

@kitzberger what about you Philipp?
are you ready to merge?

@sabbelasichon sabbelasichon merged commit 7a203dd into sabbelasichon:main Feb 5, 2024
16 checks passed
sabbelasichon pushed a commit that referenced this pull request Feb 5, 2024
* [FEATURE] Provide rule to migrate v:format.json.encode

* [FEATURE] Provide rule to migrate v:l

* [FEATURE] Provide rule to migrate v:variable.set

* [FEATURE] Provide rules to migrate v:uri.image and v:media.image

* [FEATURE] Provide rules to migrate v:or

* Added unit tests for vhs migrations

* Tweak replace v:varible.set

* Only migrate the simple cases by the 'strict' pattern
* Print warning about left over cases

* [FEATURE] Provide rule to migrate v:extension.path.relative
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants